-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KTOR-4323: WebSockets use custom serializer #3000
base: main
Are you sure you want to change the base?
Conversation
|
||
import io.ktor.websocket.* | ||
|
||
public interface SerializableWebSocketSession : WebSocketSession { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name is misleading. the session itself is not serializable, but has a serializer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's true. I didn't found a better name through, maybe WebSocketSessionWithSerializer
or WithConverter
?
|
||
@InternalAPI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this annotation is needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I use writeFrame(Frame.Close())
, which I copied from other tests.
client.webSocket("$TEST_WEBSOCKET_SERVER/websockets/echo") { | ||
repeat(TEST_SIZE) { | ||
val originalData = Data("hello") | ||
sendSerialized(originalData, DataSerializer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the use case of passing serializer here? can't it be registered in module or through @UseSerializers
annotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not work, if you need to serialize 3rd classes without the possibility to change them. @UseSerializers requires an annotation, and the SerializersModule
for generic classes requires @contextual annotation too, according to the docs. It is also not possible to serialize Any
with the overloads only, but it is with a custom serializer. That's the problem :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think something like this may be useful for regular requests as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, isn't it currently possible? I tried automatic content negation never before, with web sockets it is my first usage :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add it in another PR and focus this PR on Websockets only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: have you tried to register external serializer for 3rd party class via SerializersModule
via contextual
call?. According to code it should just work by fetching serialiaer by KClass
Or can you at least provide an example of what you want to archive, and it fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deleted my old reproducer and at the moment reproducing the case does not work. But adding all possibilities via contextual
or polymorphic
to the serializer module is annoying, so passing the serializer directly at usage is nicer IMHO. And the implementation is quite forward for web sockets.
private val json = Json {
serializersModule = SerializersModule {
val s = Main.M.serializer(Int.serializer()) as KSerializer<Main.M<*>>
polymorphic(Main::class, Main.M::class, s)
}
}
fun main() {
val main = Main.M(42)
println(json.encodeToString(main))
println(json.encodeToString(Main.M.serializer(String.serializer()), Main.M("foo")))
}
@Serializable
sealed class Main {
@Serializable
data class M<S>(val value: S) : Main()
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that both ways are little annoying :)
But, even it's quite forward for websockets, looks reasonable to add the same to http endpoints, but there it will be not such easy.
And the main my concern about, why it's not good - calling this method will fail, if there is non kx.serialization converter.
Or f.e. if there will be some custom converter, which delegates to kotlinx.serialization converter, but uses another encoding for some types. So using serializersModule
will work in both cases, but providing explicit serializer will work only in case of plain kx.serialization converter - such an inconsistency isn't so good in my opionion.
And in this specific use case, may be it will be much easier to not use this plugin at all, and do serialization handling manually, or write a simple plugin for your use case? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more questions
|
||
import io.ktor.websocket.* | ||
|
||
public interface WebSocketSessionWithContentConverter : WebSocketSession { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW: why this interface is needed, for me it's a little strange TBH
I see why it's needed, as converter for client and server are retrieved in different ways.
But IMO, even if add explicit serializers support, better to just add extensions in server and client modules for serialization with explicit serializer parameter as with current (
ktor/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/websocket/ClientSessions.kt
Line 53 in 380762b
public suspend inline fun <reified T> DefaultClientWebSocketSession.sendSerialized(data: T) { |
Line 51 in 380762b
public suspend inline fun <reified T> WebSocketServerSession.sendSerialized(data: T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but the problem is the classpath. KSerializer is defined in kotlinx-serialiation, which is only added as dependency in ktor-shared/ktor-serialization-kotlinx. To create an extension function in the client web sockets it needs to resolve KSerializer. But the actual serialization lib (kotlinx.serialization) is an implementation detail, and not available in the core module (this is the same problem with client.get { body(foo, FooSerializer)
etc.)
One workaround: adding kotlinx.serialization core to the dependencies of client-core /server-core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I see.
Now I think, that may be some additional serialization plugin should be created for kotlinx.serialization specific usage, which will support all it's cool things, like explicit serializers provider, and reduced overhead of usage reflection over typeOf serialization retrieval. But not sure if it's really needed.
Frame.Binary(true, content) | ||
} | ||
else -> error("Unsupported format $format") | ||
} | ||
} | ||
} | ||
|
||
public suspend fun <T> WebSocketSessionWithContentConverter.sendSerialized( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO: such tightly coupling of interface to implementation isn't good for general purpose API
Subsystem
Client and Server, Websockets with Kotlinx serialization
Motivation
It is not possible to use WebSockets with a custom KSerializer
Solution
Added a KSerializer as a parameter